-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create best practise guidelines #2
base: master
Are you sure you want to change the base?
Conversation
@thibautjombart @zkamvar Can you review &/| approve please? |
On it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for this! Only a couple of comments and typos.
best_practise.Rmd
Outdated
# Package contents | ||
## README | ||
- Ensure the use of non-RECON user friendly language, try not to refer to internal processes etc. | ||
- Incude a clearly defined workflow and justification for using the package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Include
best_practise.Rmd
Outdated
- Internal functions should be tagged as so | ||
- Commented out code should be removed to improve readability. | ||
- All functions both internal and external should have roxygen documentation | ||
- As a minimum internal functions should include a context e.g. #' This function is designed to... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: one white space too many
- Functions should be clearly commented | ||
- Internal functions should be tagged as so | ||
- Commented out code should be removed to improve readability. | ||
- All functions both internal and external should have roxygen documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For internal function, can you precise if you mean full roxygen doc (e.g. incl examples), or just @param
?
best_practise.Rmd
Outdated
- All functions both internal and external should have roxygen documentation | ||
- As a minimum internal functions should include a context e.g. #' This function is designed to... | ||
- Avoid using personal/colloquial language when writing comments and context - non-RECON users need to understand too. | ||
- Examples for functions should appear in the roxygen comments rather than the README |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand: are you discouraging worked examples from the README
? I thought these were useful for people who wanted to get an idea of the package's main features at a glance, but happy to change.
## Quality checks | ||
Use packages like {devtools}, {lintr}, and {goodpractice} to ensure your code passes quality checks, correct spellings, and meets common formatting standards. | ||
|
||
```r |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! :)
best_practise.Rmd
Outdated
- Examples for functions should appear in the roxygen comments rather than the README | ||
|
||
## Tests | ||
- Tests should be written prior to the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If easy, could you provide a link to some basics principles of TDD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made changes per @thibautjombart's comments
devtools::spell_check() | ||
``` | ||
|
||
# Collaboration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe a section on branch protection is missing, so I've taken the liberty to add it here:
# Collaboration | |
# Collaboration | |
## Branches | |
The master branch should represent a stable state in the repository and should be protected from direct pushes and force-pushes using GitHub's [branch protection rules](https://help.github.com/en/github/administering-a-repository/about-protected-branches). Contributions to the master branch should only be made via Pull Requests with the exception of the initial few commits before user testing. | |
Pull requests should pass quality checks from continuous integration before merging. |
One more thing regarding this: it should mesh with the guidelines found on the website: https://www.repidemicsconsortium.org/resources/guidelines/ |
@zkamvar incorporated 👍 |
Best practise document for new and existing packages - issue #1